Skip to content

server: fix checking disk offering access for snapshot volume#3791

Merged
yadvr merged 1 commit into
apache:4.13from
shapeblue:fix-snapshot-volume-npe-3783
Jan 31, 2020
Merged

server: fix checking disk offering access for snapshot volume#3791
yadvr merged 1 commit into
apache:4.13from
shapeblue:fix-snapshot-volume-npe-3783

Conversation

@shwstppr
Copy link
Copy Markdown
Contributor

@shwstppr shwstppr commented Jan 2, 2020

Fixes #3783
As reported in the issue, creating volumes from pure snapshot fails with NPE. This is due to order of calls where disk offering access is checked before checking disk offering value. This PR fixes the same.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

How Has This Been Tested?

@shwstppr
server: fix checking disk offering access for snapshot volume
c895d11 
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@shwstppr shwstppr mentioned this pull request Jan 2, 2020
Closed
5 tasks
@DaanHoogland
Copy link
Copy Markdown
Contributor

DaanHoogland commented Jan 3, 2020

you didn't need to open a new PR @shwstppr . you can force push to your own branch and then change the base branch on your PR.

@shwstppr
Copy link
Copy Markdown
Contributor Author

shwstppr commented Jan 8, 2020

@DaanHoogland didn't know that, will keep in mind for future. Thanks

DaanHoogland
DaanHoogland approved these changes Jan 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self quoting: "... the bug existed IMHO because of the method being 200 lines long and thus illegible. I will let this pass to honour history but if you have time please factor out the method contents to a helper class or a set of helper methods?"
the code is fixed by moving check access on the offering behind a null check and creating an offering on the fly. This method is not maintainable as there are more and needs TLC/refatcorring

@andrijapanicsb
Copy link
Copy Markdown
Contributor

andrijapanicsb commented Jan 9, 2020

Milestone for this one?

@DaanHoogland DaanHoogland added this to the 4.13.1.0 milestone Jan 9, 2020
@DaanHoogland DaanHoogland changed the title [WIP] server: fix checking disk offering access for snapshot volume server: fix checking disk offering access for snapshot volume Jan 10, 2020
@yadvr
Copy link
Copy Markdown
Member

yadvr commented Jan 28, 2020

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 28, 2020

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 28, 2020

Packaging result: ✖centos6 ✖centos7 ✖debian. JID-668

@yadvr yadvr closed this Jan 29, 2020
@yadvr yadvr reopened this Jan 29, 2020
@yadvr
Copy link
Copy Markdown
Member

yadvr commented Jan 29, 2020

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 29, 2020

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 29, 2020

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-686

@DaanHoogland
Copy link
Copy Markdown
Contributor

DaanHoogland commented Jan 29, 2020

@blueorangutan test

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 29, 2020

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 29, 2020

Trillian test result (tid-840)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30714 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3791-t840-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_password_server.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 75 look OK, 2 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_02_redundant_VPC_default_routes Failure 242.80 test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers Failure 155.49 test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics Failure 117.72 test_vpc_redundant.py
test_05_rvpc_multi_tiers Failure 217.35 test_vpc_redundant.py
test_05_rvpc_multi_tiers Error 246.86 test_vpc_redundant.py
test_01_vpc_site2site_vpn_multiple_options Error 295.98 test_vpc_vpn.py

@DaanHoogland
Copy link
Copy Markdown
Contributor

DaanHoogland commented Jan 30, 2020

I don't see a relation of the errors with the change, @rhtyd @shwstppr ?

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Jan 31, 2020

Agree @DaanHoogland

@yadvr yadvr merged commit a718746 into apache:4.13 Jan 31, 2020
ustcweizhou pushed a commit to ustcweizhou/cloudstack that referenced this pull request Feb 28, 2020
@shwstppr @ustcweizhou
server: fix checking disk offering access for snapshot volume (apache…
ae47330 
…#3791)

Fixes apache#3783
As reported in the issue, creating volumes from pure snapshot fails with NPE. This is due to order of calls where disk offering access is checked before checking disk offering value. This PR fixes the same.

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr approved these changes

@DaanHoogland DaanHoogland DaanHoogland approved these changes

@borisstoyanov borisstoyanov Awaiting requested review from borisstoyanov

Assignees

No one assigned

Labels

component:local-storage type:bug

Projects

None yet

Milestone

4.13.1.0

Development

Successfully merging this pull request may close these issues.

6 participants

@shwstppr @DaanHoogland @andrijapanicsb @yadvr @blueorangutan @svenvogel